Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes an issue where offset capture of a multi-field struct was incor… #251

Merged
merged 3 commits into from
Jun 16, 2021

Conversation

rgantt
Copy link
Contributor

@rgantt rgantt commented Jun 16, 2021

Fixes an issue where offset capture of a multi-field struct was incorrectly returning field name positions instead of value positions

Description of changes:

This adds a condition to the resetting of the value_start position during the _next operation to bring the behavior of the text reader in line with the binary reader when doing offset capture.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…rectly returning field name positions instead of value positions
@rgantt rgantt requested a review from tgregg June 16, 2021 04:59
…ead field names when moving to a position within a struct
Copy link
Contributor

@tgregg tgregg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, minor comment below.

Comment on lines 756 to 758
// Easy assertions: there's only one value, "value," and we should have read it both times
assertStringsEqual((char *)value.value, cread_val1, strlen(cread_val1));
assertStringsEqual((char *)value.value, cread_val2, strlen(cread_val2));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably good enough, but I would feel better about it if the values weren't the same. That way we know this won't happen to pass if the second seek somehow leaves the reader positioned at the first value.

@tgregg tgregg merged commit 799c4d4 into amazon-ion:master Jun 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants